-
Notifications
You must be signed in to change notification settings - Fork 599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/add boto3 sqs instrumentation #1081
Feature/add boto3 sqs instrumentation #1081
Conversation
Can we not extend existing sqs instrumentation? We shouldn't be publishing two instrumentation packages for the same library. This will cause a lot of confusion. We'll be putting additional burden on users to figure out which one to use. What happens if both instrumentations are installed? Will both work? Will one cancel the other? Will the cancellation be deterministic? Even if the answer is a No today, further chances to one lib could end up breaking the other. Personally if we think this implementation is far superior and it'd be harder to add these features to existing one, I'd rather completely replace the existing one with this instead of creating a new competing package. |
I see this is for |
I'll review the code in more detail shortly but at first glance, any reason we don't follow same pattern as boto instrumentation where we have a single boto/botocore package with extensions for things like sqs and s3? |
+1 to this. I personally liked how OTel JS does it where they have a single instrumentation package (they actually have one pkg both v3 and v2, but it's fine that we how one for They have a
and their file organization by service looks very clean too. It would be nice for users to have As another point, what about calling this |
I would really love for it to have been like this. Unfortunatly, as I state in the documentation, the SQS classes are loaded in runtime from a template via the
We can create an
|
It sounds like something is wrong with our packaging system. Ideally it shouldn't matter whether a something is a sub-module or an entirely different package. Could you share more details about the issues you ran into when trying to organize this as a single package? |
By single package, I mean |
@@ -0,0 +1,63 @@ | |||
# Copyright The OpenTelemetry Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not require a separate file. you can add the same test class in test_boto3sqs_instrumentation.py only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -0,0 +1,44 @@ | |||
# Copyright The OpenTelemetry Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as test_getter.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
).__getitem__(*args, **kwargs) | ||
if not isinstance(retval, dict): | ||
return retval | ||
if not (receipt_handle := retval.get("ReceiptHandle", None)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could create a problem in python < 3.8 as walrus operator (':=') is available only from python 3.8. Its better to make it more generic by separating assignment and checking of receipt_handle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad. For some reason, I remembered it was a python 3.6 feature. Changed
|
||
@staticmethod | ||
def _safe_end_processing_span(receipt_handle: str) -> None: | ||
if started_span := Boto3SQSInstrumentor.received_messages_spans.pop( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before for walrus operator (':=')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
links=links, | ||
kind=SpanKind.CONSUMER, | ||
) | ||
with trace.use_span(span): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine start_span and use_span in one API: start_as_current_span() with with statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Done
attributes = kwargs.pop("MessageAttributes", {}) | ||
propagate.inject(attributes, setter=boto3sqs_setter) | ||
retval = wrapped(*args, MessageAttributes=attributes, **kwargs) | ||
if message_id := retval.get("MessageId"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can pass message_id in enrich_span() so that you dont have to repeat the line 242 and 243 again as its already present in enrich_span()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to add it later, because if the call of retval = wrapped(*args, MessageAttributes=attributes, **kwargs)
will raise (by a wrong queue for example), the span will still have the available information - helping the user to trace and debug. @sanketmehta28 WDYT?
message_identifier = successful_messages["Id"] | ||
if message_span := ids_to_spans.get(message_identifier): | ||
if message_span.is_recording(): | ||
message_span.set_attribute( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before for message_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to this case I add the information after because if I set all the information after, a failure will create a span with lacking information that can be provided.
Description
This PR instruments the SQS client as part of python's boto3 package. Unlike the instrumentation of boto3, this instrumentation propagates the context and baggage over the sent messages, following the messaging systems spec.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.